feat(Select): Typeahead example #10207
Conversation
|
Preview: https://patternfly-react-pr-10207.surge.sh A11y report: https://patternfly-react-pr-10207-a11y.surge.sh |
adamviktora
changed the title
There was a problem hiding this comment.
hover on MenuItem and keep the cursor there, then move in the menu via arrows up/down -> the originally hovered item will still have the hovered styling (which is not ideal when the hover and focus styling looks the same)
This may be more on the user rather than it being something for us to resolve.
| @@ -97,6 +97,7 @@ class MenuToggleBase extends React.Component<MenuToggleProps> { | |||
| aria-expanded={isExpanded} | |||
| onClick={onClick} | |||
| aria-label={ariaLabel || 'Menu toggle'} | |||
| tabIndex={-1} | |||
There was a problem hiding this comment.
Was the main issue with being able to focus and click the toggle via keyboard the focus/styling issue?
Just wondering if it'd be confusing that this toggle can be interacted with via mouse and keyboard when using a screen reader, but not via keyboard only.
There was a problem hiding this comment.
The main issue was that it focused the first menu item after focusing and clicking the toggle with keyboard, so then user is not able to write text to the input. We always want to have the input focused I think.
I am checking the w3 combobox example for reference: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-autocomplete-list/ and the toggle button also has a tabindex="-1"
We also might change the toggle icon to arrow-up when the menu is expanded.
There was a problem hiding this comment.
Sounds good, we can keep this in and see if anyone brings it up and revisit it then if so.
Changing the toggle icon would probably be worth a discussion since it'd have to include updating anywhere else we're using icons in a similar fashion, as well as making sure we have documentation about consumers needing to do the same if they're doing something more custom.
adamviktora
force-pushed
the
select-typeahead-example-2
branch
from
3ab72d1
to
9800074
Compare
There was a problem hiding this comment.
Had more of a nit below that would be great to get others' feedback on, but otherwise things are looking nice. If we'd need to also update other Typeahead examples I think it'd be worth doing so in this PR rather than needing to do a followup for something we're already starting to fix
| ]; | ||
| resetActiveAndFocusedItem(); |
There was a problem hiding this comment.
This isn't a blocker and more of a nit, but it is odd what you had mentioned about the focus styling for the "no results" option.
We could replace this reset... call and instead setActiveAndFocusedItem to keep focus on the first item regardless. Or we could maybe try to combine the 2 useEffects into a single one to better ensure focus never gets placed on a "no results" option.
Another option is to just not auto place the "focus" on the first item, which is something that a W3C combobox example is doing. Which that actually makes sense when testing with VoiceOver. As I type into the input, I may not get any indication that the first item is "focused", so if I press down arrow to traverse the list it sounds like I'm "starting" on the second item. Sometimes I can hear VO saying "[first item] not selected...", but sometimes it may not do that or it may get cutoff as it announces what I've typed into the input. Preventing the "autofocus" would mean pressing an arrow key brings me to the expected first (or last, if Up arrow is pressed) item. Curious what others think, though.
There was a problem hiding this comment.
I really like the other option you are mentioning about not auto focusing the first option. As you say, it makes sense with the voice over, and it would also solve the unwanted focusing of the "no results" option.
If we would go with this option, we should then prevent selecting the first option on pressing "enter" - it should only open/close the menu.
About updating other examples - I agree with that, I'll update them once we agree on the final version of the Typeahead behaviour.
There was a problem hiding this comment.
Eric and I were talking about this earlier, and I agree with both of you that it would probably make sense to not auto focus the first option and just shift "focus" to the first option when the user presses the arrow keys (which I believe is what the W3C combobox example is doing). This would probably help improve the screen reader experience.
- new changes were done also based on SelectTypeahead example updates (patternfly#10207)
|
|
||
| const onSelect = (_event: React.MouseEvent<Element, MouseEvent> | undefined, value: string | number | undefined) => { |
There was a problem hiding this comment.
When selecting a creatable option for a single select, the new option is created + selected, but the menu stays open. I think I'd expect the menu to close after selection, similar to selecting an already existing option.
There was a problem hiding this comment.
Yeah, I know, it was intentional. I thought it might be better for user to see the option was actually created, so I kept the menu open. But I was hesitating on this one. I'll change that so it is consistent with selecting other options.
It is not that much important here, but for the templates, we might want to have this behavior customizable with props.
There was a problem hiding this comment.
Ah gotcha! With that context if we did want to keep the menu open, it'd be worth adding some verbiage in the example description just to state that. Something like "Unlike other typeahead examples, this example keeps the select menu open after selecting a creatable option in order to show that the item was created." Bascially just so consumers don't think we're recommending that this be the behavior they implement or something (though if they need that behavior they of course can keep it).
There was a problem hiding this comment.
OK, good idea. Anyway, I won't change that back anymore, the closing after creation seems more convenient.
* fix(SelectTypeahead example): make "no results" option aria-disabled * fix(SelectTypeahead example): don't close the menu on input click when there is text * fix(SelectTypeahead example): remove visual focus on item after closing the menu Prevents situation where we open the menu via focusing on the toggle arrow and clicking enter -- then two items can have focus styling, which is not ideal. * fix(SelectTypeahead example): remove check icon from the selected option when input text changes * fix(SelectTypeahead example): rename example * feat(Select): add prop to opt out of focusing first menu item on open Flag prop shouldFocusFirstMenuItemOnOpen has been added, because of typeahead select, which should keep focus on the input. * refactor(SelectTypeahead example): adaption on first menu item focused * feat(MenuToggle): make typeahead toggle button not focusable * fix(SelectTypeahead example): focus input after toggle button click * feat(SelectTypeahead example): change the focused item on hover * fix(SelectTypeahead example): don't focus on first item after tabbing * feat(Select): add typeahead select template * fix(SelectTypeahead): address PR review - new changes were done also based on SelectTypeahead example updates (#10207) * fix(SelectTypeahead template): call onToggle every time menu opens/closes * refactor(SelectTypeahead template)
- does not apply visual focus on the first menu option - handles disabled options - opens menu on pressing up/down arrow keys
…at exact option exists
adamviktora
force-pushed
the
select-typeahead-example-2
branch
from
f2a551e
to
fa9a1af
Compare
|
I renamed the prop It was unfortunately merged with the longer name in this PR: #10235, but as it was not released yet, I think it is safe to rename it back without it being a breaking change. |
* fix(SelectTypeahead example): make "no results" option aria-disabled * fix(SelectTypeahead example): don't close the menu on input click when there is text * fix(SelectTypeahead example): remove visual focus on item after closing the menu Prevents situation where we open the menu via focusing on the toggle arrow and clicking enter -- then two items can have focus styling, which is not ideal. * fix(SelectTypeahead example): remove check icon from the selected option when input text changes * fix(SelectTypeahead example): rename example * feat(Select): add prop to opt out of focusing first menu item on open Flag prop shouldFocusFirstMenuItemOnOpen has been added, because of typeahead select, which should keep focus on the input. * refactor(SelectTypeahead example): adaption on first menu item focused * feat(MenuToggle): make typeahead toggle button not focusable * fix(SelectTypeahead example): focus input after toggle button click * feat(SelectTypeahead example): change the focused item on hover * fix(SelectTypeahead example): don't focus on first item after tabbing * feat(Select): add typeahead select template * fix(SelectTypeahead): address PR review - new changes were done also based on SelectTypeahead example updates (patternfly#10207) * fix(SelectTypeahead template): call onToggle every time menu opens/closes * refactor(SelectTypeahead template)
* refactor(Select): rename shouldFocusFirstMenuItemOnOpen * feat(SelectTypeahead example): better arrow up/down keys handling - does not apply visual focus on the first menu option - handles disabled options - opens menu on pressing up/down arrow keys * feat(SelectTypeahead example): don't close menu on clicking clear button when open * refactor(SelectTypeahead example) * refactor(SelectTypeahead example) * fix(SelectTypeaheadCreatable example): changes based on SelectTypeahead * fix(SelectMultiTypeahead example): changes based on SelectTypeahead * fix(SelectTypeaheadCreatable example): don't show create option if that exact option exists * fix(SelectMultiTypeaheadCreatable): changes based on SelectTypeahead * fix(SelectMultiTypeaheadCheckbox): changes based on SelectTypeahead * fix(SelectTypeaheadCreatable): close menu after creating option * fix(SelectTypeahead template): rename prop back to shouldFocusFirstItemOnOpen
* feat(Select): Typeahead template (#10235) * fix(SelectTypeahead example): make "no results" option aria-disabled * fix(SelectTypeahead example): don't close the menu on input click when there is text * fix(SelectTypeahead example): remove visual focus on item after closing the menu Prevents situation where we open the menu via focusing on the toggle arrow and clicking enter -- then two items can have focus styling, which is not ideal. * fix(SelectTypeahead example): remove check icon from the selected option when input text changes * fix(SelectTypeahead example): rename example * feat(Select): add prop to opt out of focusing first menu item on open Flag prop shouldFocusFirstMenuItemOnOpen has been added, because of typeahead select, which should keep focus on the input. * refactor(SelectTypeahead example): adaption on first menu item focused * feat(MenuToggle): make typeahead toggle button not focusable * fix(SelectTypeahead example): focus input after toggle button click * feat(SelectTypeahead example): change the focused item on hover * fix(SelectTypeahead example): don't focus on first item after tabbing * feat(Select): add typeahead select template * fix(SelectTypeahead): address PR review - new changes were done also based on SelectTypeahead example updates (#10207) * fix(SelectTypeahead template): call onToggle every time menu opens/closes * refactor(SelectTypeahead template) * feat(Select): Typeahead example (#10207) * refactor(Select): rename shouldFocusFirstMenuItemOnOpen * feat(SelectTypeahead example): better arrow up/down keys handling - does not apply visual focus on the first menu option - handles disabled options - opens menu on pressing up/down arrow keys * feat(SelectTypeahead example): don't close menu on clicking clear button when open * refactor(SelectTypeahead example) * refactor(SelectTypeahead example) * fix(SelectTypeaheadCreatable example): changes based on SelectTypeahead * fix(SelectMultiTypeahead example): changes based on SelectTypeahead * fix(SelectTypeaheadCreatable example): don't show create option if that exact option exists * fix(SelectMultiTypeaheadCreatable): changes based on SelectTypeahead * fix(SelectMultiTypeaheadCheckbox): changes based on SelectTypeahead * fix(SelectTypeaheadCreatable): close menu after creating option * fix(SelectTypeahead template): rename prop back to shouldFocusFirstItemOnOpen * feat(Dropdown): Added simple template (#10308) * feat(Dropdown): Added simple template * Added tests * Added imports to example file * Additional fixes for docs fail * Updated import name * Added additional tests * feat(templates): toggle props & improvements (#10473) * feat(templates): toggle props & improvements * remove toggleContent from typeahead template * update template names * update tests * added SimpleSelect tests * fix yarnlock * fix(): update demo-app version and snap --------- Co-authored-by: adamviktora <[email protected]> Co-authored-by: Eric Olkowski <[email protected]>
What: Closes #10180
shouldFocusFirstItemOnOpeninSelect, because typeahead would auto focus on the first item (instead of keeping the focus on the input)tabindex={-1}to theMenuTogglearrow button for typeahead variant to prevent being able to use typeahead as a classic select and focus items (I wonder if this might break some consumer's tests and we should warn them?)Additional issues: